@Shootfast Does this look reasonable to you? I believe you were the last person in the raw code.

Hey Larry,

I actually have some more patches around this issue coming in the near future, but if this fixes an immediate problem then I don't have an issue with it being merged.

@Shootfast As I look at it a bit more, I believe also that the recent code you added for the non-demosaic case does not behave well for rotated images. Now I'm wondering if I should commit this small PR now as-is (which at least fixes the case for non-rotated images) and then throw the rest into your lap, since (a) you know this code better than I do and can probably fix it more expediently, (b) you may well have already fixed it on your end if you have in-progress changes, and (c) I don't want to stomp on the same lines and make whatever you're working on be a difficult merge for you.

What do you think? Is this slated in the near enough future for you that I should turn the rest over to you?

Looking into this a bit further, I'm not sure this is correct.
The raw sensor data often contains "junk" pixels, that aren't intended to be a part of the final image.
I was taking the effort to trim these off here.
I had tested with a CR2 image in all orientations, but perhaps the behaviour is different for other formats?

I wonder if keeping the junk pixels should be exposed as another read option?

If you're not confident about this, would it be better for me to abandon this PR and you can just be sure to tackle the correct unmosaiced+rotated combinations in the next set of changes you were working on?

@Shootfast Ping -- Would you like me to merge this partial fix, or will that just make life harder for you and I should let you fold the more correct fixes into your next PR in the raw code?

If you don't mind waiting a little bit, I'd prefer if you could hold off on the partial fix. I've worked a bit more at this over the weekend, but still not quite ready to push anything your way.

No, I don't mind the delay, I'd rather get a more complete fix and I lack the time and libraw experience to do it efficiently.

Looking into this a bit further, I'm not sure this is correct. The raw sensor data often contains "junk" pixels, that aren't intended to be a part of the final image.

What needs to change with this patch to make it more correct? I'm somewhat frequently running into CR2/CR3 issues in particular and would rather just apply a quick patch if the larger fix will take longer, but of course I don't want to break anything else silently.

@AdamMainsTL Good question.

@Shootfast What's your opinion here? If this modest patch a step forward (even though not a complete solution), or does it introduce as many new problems as it fixes? Back in October you made it seem like you were imminently going to submit a more comprehensive set of changes. Is that ready to go?

Ah sorry, yes, it's pretty close to done. Have just been distracted with other things. Let me take another look and upload my work in progress somewhere for you to take a look at!

I've pushed my commits to here and rebased them off the latest master:
https://github.com/Shootfast/oiio/tree/raw_features

Would you be able to try it out? The "junk" pixels are now maintained in the overscan areas of the image. This is so they are maintained when they are re-written out via the new raw DNG writer.

The DNG writer is still WIP, but the raw reader changes are probably mergeable if you don't see any errors in your files.

Thanks for the update @Shootfast! I will be able to test these changes probably earliest on Monday or so, but I'll let you know if it fixes our problem cases.

@Shootfast -- Not directly related to this issue ticket, but in glancing at your dng output, I had a couple of thoughts for you:

1. I get the feeling that you may have cribbed material from the TIFF output some time ago and it has diverged. In particular, it doesn't contain any provision for IOProxy support. You may wish to re-synchronize and/or otherwise make sure to get the proxy support in place, as is coming to be expected from all plugins these days.

2. It looks like you're just writing a compliant TIFF file, using an altered version of tiffoutput.cpp. So I can't help but wonder if, in the grand scheme of things, it would simply be better to implement DNG in the actual TIFF output (just checking the filename extension, and in the case of dng, doing, or not doing, the various things that distinguish a true DNG from a generic TIFF. I'm just thinking that maybe there would be less code duplication this way, and easier to simultaneously make improvements to both tif and dng output at the same time, since they will mostly be literally the same code. (And conversely, if we ever support output of other "camera raw" formats, they will be wholly unrelated to the TIFF/DNG code here.) I mean, you be the judge of the best way to do all this, perhaps I'm wrong and this has so little overlap (despite using libtiff) with the generic TIFF that it's more trouble than it's worth to combine them.

1. Yeah I can totally revisit and add IOProxy support. This code isn't quite ready just yet anyways. Still a fair bit to do.
2. It has been rather difficult to trick libtiff into writing a compliant DNG, and I think it would be even harder to follow the logic if it were entwined with the regular TIFF writer. That said there is certainly a lot of code that could probably be shared, so I was hoping I could refactor them both and share functions at some point. I don't know how likely another raw format writer would be, and if there was I assume it would probably be better off under another plugin name.

It has been rather difficult to trick libtiff into writing a compliant DNG, and I think it would be even harder to follow the logic if it were entwined with the regular TIFF writer.

That's a very compelling rationale. Mostly I just wanted you to know that the option was open to you if it seemed simpler to combine them.

I finally got around to trying to test this, but unfortunately I'm unable to build it with my build process due to boost errors. I'm not fully sure whether this is due to how I have things setup (Conan with some CMake patches), or something with this branch. Here are build logs for reference: https://gist.github.com/AdamMainsTL/14204073a761a512170a173ed33958e0

What's strange to me is that I can build 2.3.13.0 with the same process just fine which has the same Boost include syntax, so unsure what's causing it to "find" it, but then not find it when linking. I've also been using this same process since around 2.2.9.0, so this break is new.

Edit: I've been able to resolve this now. I thought that current master branch was what 2.3.13 was based on, but it seems like that's not the case. I took a look at the 2.3.13 source and patched the CMake and it is building now. Will be able to test it later today.

Edit again: Turns out that the build process doesn't like the master branch. Might actually not be able to test it today.

So I've finally gotten it working after changing Boost and OpenEXR linking stuff in the CMake files. I also had to patch rawoutput.cpp because clang doesn't like narrowing int to uint32_t in initializer lists (error log)

So far it seems like the current code does not address #3115 as all the example files in that post still crash for me with this build. The NEF and PEF might be fine and crash in other code, but the 4 other ones for sure are crashing inside of read_image.

Ah sorry, I've dived into this further and found the root cause for these other failures. Short version is that the libraw API does a terrible job of presenting the data inside a RAW file :(

Thus far I'd relied on the rawdata.raw_image pointer being set to point me at the bayer patterned footage. Unfortunately, if the RAW file uses a different underlying data type (ie not single channel bayer), then libraw actually jumps between a number of different pointers (with only one of them being set at a time).

I need to take a closer look, as some of these pointers are also completely different data types. The shortlist is:

/* alias to single_channel variant */
ushort *raw_image;
/* alias to 4-channel variant */
ushort (*color4_image)[4];
/* alias to 3-color variand decoded by RawSpeed */
ushort (*color3_image)[3];
/* float bayer */
float *float_image;
 /* float 3-component */
float (*float3_image)[3];
/* float 4-component */
float (*float4_image)[4];

Your example images actually go through a number of these, so they are a good test list. Apologies that I forgot you had already sent them when I uploaded my last version of the code.

I'm glad the examples are helpful. I hope it's not too hard of an issue to work around. If you get another update let me know and I can test it out with some other files that crash on my end.

Thanks for continuing to work on this!

@Shootfast Hey, just wanted to know if there's any update for this? Not sure if I can help, but does this change need some?